Test not merge: enable pushdown_filters by default#23369
Draft
zhuqi-lucas wants to merge 3 commits into
Draft
Conversation
Flip the default of datafusion.execution.parquet.pushdown_filters from false to true, so Parquet RowFilter (within-row-group late materialization) is enabled out of the box. Draft to gauge the current ClickBench regression picture with recent arrow-rs improvements (adaptive RowSelection representation in apache/arrow-rs#8733) and DataFusion's TopK dynamic RG pruning (apache#22450) already landed. Related: apache#3463, apache#20324, apache#22883, apache#23263.
zhuqi-lucas
changed the title
Contributor
Author
|
run benchmark clickbench_partitioned |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing qizhu/enable-pushdown-filters-by-default (114dbc0) to 32e1146 (merge-base) diff using: clickbench_partitioned File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usageclickbench_partitioned — base (merge-base)
clickbench_partitioned — branch
File an issue against this benchmark runner |
Contributor
|
Thanks for this @zhuqi-lucas I think the current feeling is that to reduce these regressions we will need an adaptive approach. For example this from @adriangb |
Contributor
Author
Thanks @alamb , i can help this epic and testing something related approach. |
Add a plan-time heuristic in ParquetSource::try_pushdown_filters that declines to push filters into the scan when the projection contains fewer than PUSHDOWN_MIN_NON_FILTER_COLS (currently 3) columns not referenced by the filter. Rationale: RowFilter has fixed per-row machinery overhead that only pays for itself when the wide-column decode it lets us skip is large. When the filter references most of the projection, that saving is small and the overhead dominates — see the Q10/Q40/Q41 ClickBench regressions discussed in apache#23263. When the heuristic declines: pushdown_filters is treated as disabled for this scan, so the FilterExec stays above the scan (correctness preserved) and the predicate is still injected into ParquetSource for stats / bloom / page-index pruning. Local numbers (5 iters, 12 partitions): Q10 (non-filter=1): 95ms with push → 82ms with gate (regression fixed) Q11 (non-filter=2): 116ms with push → 73ms with gate (regression fixed) Q41 (non-filter=6): 20ms with push → 11.5ms with gate (regression fixed) Q22 (non-filter=1): 835ms with push → 1150ms with gate (win lost — heuristic misses this) Q23 (non-filter=104): 318ms with push, still pushed with gate (kept)
Contributor
Author
|
run benchmark clickbench_partitioned |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing qizhu/enable-pushdown-filters-by-default (ab8fbb9) to 7ff7278 (merge-base) diff using: clickbench_partitioned File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usageclickbench_partitioned — base (merge-base)
clickbench_partitioned — branch
File an issue against this benchmark runner |
Contributor
Author
|
CI ClickBench numbers on this PR experiment — cc @alamb @adriangb. Change: flip
Basically no regression, and it seems resolve the pushdown_filters regression, but Adaptive predicate evaluation should be better. |
Contributor
|
@zhuqi-lucas I wanted to chime in here that we can also in future add a auto mode here that works on % of columns to detect if we want to push down or not. |
Contributor
@adriangb (and others) also are working on runtime detection of usefulness/selectivity of (row) filters, hopefully that will help as well |
Contributor
Author
Yeah @RatulDawar , @Dandandan , runtime detection of usefulness/selectivity of (row) filters is in progress. |
Contributor
|
run benchmark clickbench_partitioned |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing qizhu/enable-pushdown-filters-by-default (ab8fbb9) to 7ff7278 (merge-base) diff using: clickbench_partitioned File an issue against this benchmark runner |
Contributor
This seems like a very nice find (a simple heuristic) @zhuqi-lucas -- would you be willing to make a new PR with just this change? I think we should consider doing it. The heuristic might also want to take into account data type (e.g. projecting string columns might count more than projecting an int column) I would like to run some more tests to see that it works across other query benchmarks, but I think we should do it |
Contributor
Author
Thanks @alamb! Yes, let me open a separate PR with just the heuristic gate so it can be reviewed and merged on its own, then follow up with the pushdown_filters=true default flip once we have the extended benchmark data. Good call on data-type weighting — I'll experiment with weighting non-filter columns by an estimated byte size per row (e.g. Utf8 / Binary / List count much more than Int32) so the gate captures the actual wide-decode saving. Will also make the threshold configurable so it's tunable per workload if the heuristic misfires on someone's data. I'll post the new PR shortly! |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usageclickbench_partitioned — base (merge-base)
clickbench_partitioned — branch
File an issue against this benchmark runner |
Contributor
Let's not go too crazy with tuning knobs as we are hoping for something more dynamic in the future What I think we can defend easily is "a simple heuristic that works for most workloads" If we make the heuristic too complicated then we risk it overfitting / being hard to tune and understand I think we should save our compleixty budget for the adaptive behavior |
Contributor
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Part of #3463 and #20324. Draft only — putting this up to gauge the current ClickBench regression picture (I'll run the benchmark separately) now that recent adaptive machinery has landed:
Also see #22883 "Adaptive predicate evaluation" for the placement-adaptive follow-up, and #23263 "Late materialization when LIMIT prunes heavily" — where local Q23 went from 4173ms → 318ms just by flipping this config.
What changes are included in this PR?
Single-line change:
datafusion.execution.parquet.pushdown_filtersdefaultfalse→true.Nothing else touched.
configs.mdregeneration and any sqllogictest / explain-plan expectation updates will follow once we see what CI catches.Are these changes tested?
Draft — expecting CI to surface sqllogictest EXPLAIN diffs and any per-query regressions. I'll iterate based on that.
Are there any user-facing changes?
Yes — Parquet reads default to applying filter expressions during decode. Marking
api change.